Conversation
ian-noaa
left a comment
ian-noaa
left a comment
There was a problem hiding this comment.
I had a bit of feedback on the inline style in the home app. It'd be nice to stay consistent and use bootstrap classes if possible. Otherwise, everything else looks good!
| class="text-center h-100" | ||
| src="img/noaa-logo-rgb-2022.svg" | ||
| alt="NOAA Logo" | ||
| style="height: 80px;" | ||
| src="img/metexpress_transparent.png" | ||
| alt="METexpress Logo" | ||
| /> | ||
| {{ else }} | ||
| <img | ||
| style="height: 80px;" | ||
| src="img/mats_transparent.png" | ||
| alt="MATS Logo" |
There was a problem hiding this comment.
Would it be possible to use bootstrap's styling classes to specify the image size? The original logo was utilizing class=text-center h-100 to get proper image sizing.
I was trying to avoid having to specify inline CSS style directly on elements in the home app and just use Bootstrap utilities instead. Inline CSS is considered a bit of an antipattern - it's then hard to update styles globally. I was also a bit worried that if we specify CSS style= directly on elements, that we'll end up fighting bootstrap, or that we'll lose bootstrap's responsiveness.
There was a problem hiding this comment.
Otherwise, the home app does have an external stylesheet we could use - IIRC, then we'd just add another custom class.
https://github.com/NOAA-GSL/MATS/blob/main/home/static/css/home.css
There was a problem hiding this comment.
I agree about using bootstrap where we can! However the bootstrap h and w controls are pretty limited. For example, h-100 will give you 100% of the image's height, which for our four images are too large. I didn't see that the home app had a style sheet, I will gladly mode it there!
There was a problem hiding this comment.
Sounds good! And agreed about bootstrap's size controls being limited. It seems like it's 25%, 50%, 75%, and 100% of the parent's height, which may or may not be appropriate. If we did want that whole parent <div> to be 80px high, we could set the style on that element.
2 participants
This PR adds the MATS logo to MATS and the home app, and includes a METexpress logo for non-GSL installations of METexpress. It also removes some reset to defaults calls because I was planning to do that this week anyway, and they were annoying me while I was working on this.